Do you have performance numbers for this?

Performance numbers available at, as measured on:

uname -a
Linux anitu-desk 4.6.2-1-ARCH #1 SMP PREEMPT Wed Jun 8 08:40:59 CEST 2016 x86_64 GNU/Linux

cat /proc/cpuinfo
processor   : 0
vendor_id   : GenuineIntel
cpu family  : 6
model       : 60
model name  : Intel(R) Core(TM) i7-4790K CPU @ 4.00GHz
stepping    : 3
microcode   : 0x1c
cpu MHz     : 4389.218
cache size  : 8192 KB
....

Implemented requested changes :D

What if we just changed the isNaN(byteOffset) to byteOffset !== byteOffset and nothing else? That should simplify the changes while achieving the original performance goal.

I looked into that, it gets slightly lower performance(under 1% lower).
The middle ground would be something like the general case gets a free bypass and then use the original code :)

Any comments on the (typeof byteOffset === 'string') condition?

It feels really awkward :D

typeof byteOffset === 'string' is just fine since it's checking for an optional argument. That particular conditional is checking if byteOffset was not provided, but the encoding was. This can be checked only because the two parameters are different types.

So I gather I should just simplify the patch according to mscdex's comments and resend my patch

@adrian-nitu-92 ... sorry for the delay, updating this PR would be the way to do it. If you're still interested in moving this forward, simply add the new commits here and we'll take a look! :-)

Thank you for the feedback, I simplified my patch

@nodejs/buffer @nodejs/benchmarking @mscdex

CI: https://ci.nodejs.org/job/node-test-pull-request/4279/

@adrian-nitu-92 could you put a comment explaining what you are doing? Its clever, and clever often leads to misunderstanding. A quick comment should alleviate what looks like a bug.

The comment below is somewhat misleading, you could also just clean that up.

Is this still something that should happen?

I think adding a comment above the conditional might be a good idea. Either way it LGTM.

Would be worth it to run this with Number.isNaN(byteOffset). Since the operation just above it already coerces the value, should be able to do a more direct check.

@trevnorris I'm not sure it'd be worth it since it needs to be converted anyway?

@mscdex What I mean is that because of:

byteOffset = +byteOffset;  // Coerce to Number.

we can use the more strict Numer.isNaN() rather than the generic.

@trevnorris The n !== n style of NaN checking seems to be faster than Number.isNaN(n).

@mscdex Hm. Seems the code was updated to use !== between my last two comments, but I didn't look at the change before my last comment. Change looks good.

#12286 seems to duplicate this and include some other things (a benchmark file, a comment explaining why it's doing things this way, and implementing the same change where Number.isNan() is used in one other place in the file. I'm going to close this in favor of that PR, but feel free to comment or re-open if you think this should stay open. Thanks for the PR!